-
Notifications
You must be signed in to change notification settings - Fork 533
Document how closure capturing interacts with discriminant reads #1837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This is the behavior after the bugfixes in rustc PR 138961.
r[type.closure.capture.precision.discriminants] | ||
### Capturing for discriminant reads | ||
|
||
If pattern matching requires inspecting a discriminant, the relevant place will get captured by `ImmBorrow`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we mutably access the contents, right? E.g.
let c = || match x {
(Example::A(ref mut _y), _) => println!("variant A"),
(Example::B(_), _) => println!("variant B"),
};
Is that already implied by the other rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok yep, that's the "Shared prefix" thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Do you think this deserves a clarification? (if so, how would you word it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I think it's clear in the context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look accurate to me. I don't know about things related to how the reference is organized so I'll leave to someone who knows to approve this.
c(); | ||
``` | ||
|
||
Likewise, a slice pattern that matches slices of all possible lengths does not constitute a discriminant read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're talking about changing this behavior. cc @Nadrieril.
That said, I think we should merge it in this form, and then update this when we've changed this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea to change this came from the PR that is being documented by this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, can you also unwrap any new text being added?
Likewise, a slice pattern that matches slices of all possible lengths does not constitute a discriminant read. | ||
|
||
```rust | ||
let mut x: &mut [i32] = &mut [1, 2, 3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut x: &mut [i32] = &mut [1, 2, 3]; | |
let x: &mut [i32] = &mut [1, 2, 3]; |
the slice pattern needs to inspect the length of the scrutinee. | ||
|
||
```rust,compile_fail,E0506 | ||
let mut x: &mut [i32] = &mut [1, 2, 3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut x: &mut [i32] = &mut [1, 2, 3]; | |
let x: &mut [i32] = &mut [1, 2, 3]; |
Matching against a [range pattern][patterns.range] constitutes a discriminant read, even if | ||
the range matches all possible values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following this terminology, particularly around the user of the term "discriminant". My understanding is that a discriminant is specifically a tag associated with an enum. I wouldn't consider an integer to have a discriminant. Can you clarify what this means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this happened because the relevant rustc has a distinction between (a) binding sub-values to identifiers with various binding modes, and (b) discriminant reads—with other "shallow/immediate" reads, such as the length field of a slice, being shoe-horned into the category of discriminant reads.
From the perspective of reference text, this obviously doesn't make sense. I'll rephrase that as just "constitutes a read", I think, unless you have any better ideas on how to word this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me.
Matching against a [slice pattern][patterns.slice] constitutes a discriminant read if | ||
the slice pattern needs to inspect the length of the scrutinee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about the use of the term "discriminant".
This is the behavior after the bugfixes in rust-lang/rust#138961. I have successfully ran
mdbook test
withRUSTUP_TOOLCHAIN
pointed at a stage1 built on top of the aforementioned PR – until it's merged, the CI here will fail.